add additional memory access validation in seccomp.c#3
Open
mpzanoosi wants to merge 1 commit intocanonical:mainfrom
Open
add additional memory access validation in seccomp.c#3mpzanoosi wants to merge 1 commit intocanonical:mainfrom
mpzanoosi wants to merge 1 commit intocanonical:mainfrom
Conversation
function sc_must_read_filter_from_file is changed to be checked against memory length validation
shondll
suggested changes
Sep 11, 2024
| if (prog->len > MAX_BPF_SIZE / sizeof(struct sock_filter)) { | ||
| die("seccomp filter too large"); | ||
| fclose(file); // Add this line to close the file on error | ||
| return NULL; // Add this line to handle the error properly |
There was a problem hiding this comment.
The function is void, so this is usesless.
| // When reading syscall numbers or other data, ensure they are within valid ranges. This should be done in the loop where the file is being read: | ||
| if (prog->len > MAX_BPF_SIZE / sizeof(struct sock_filter)) { | ||
| die("seccomp filter too large"); | ||
| fclose(file); // Add this line to close the file on error |
There was a problem hiding this comment.
The function should not close the file by itself as it received it open already. This call will make an unexpected side effect.
| prog->filter = (struct sock_filter *)malloc(MAX_BPF_SIZE); | ||
| // When reading syscall numbers or other data, ensure they are within valid ranges. This should be done in the loop where the file is being read: | ||
| if (prog->len > MAX_BPF_SIZE / sizeof(struct sock_filter)) { | ||
| die("seccomp filter too large"); |
There was a problem hiding this comment.
die() effectively closes the whole program, so it is no return. No use of calling stuff after die().
| } | ||
|
|
||
| // Ensure any dynamically allocated memory is properly managed. For example, if you allocate memory for a buffer, make sure to free it: | ||
| // prog->filter = (struct sock_filter *)malloc(MAX_BPF_SIZE); |
There was a problem hiding this comment.
- Done leave commented out lines in production code;
- The cast was fine, you should leave it;
- Add a check for too much memory requested:
if (len_bytes > MAX_BPF_SIZE)
die(...)
| // Ensure any dynamically allocated memory is properly managed. For example, if you allocate memory for a buffer, make sure to free it: | ||
| // prog->filter = (struct sock_filter *)malloc(MAX_BPF_SIZE); | ||
| prog->filter = malloc(len_bytes); | ||
| if (prog->filter == NULL) { |
There was a problem hiding this comment.
The next check is the same, remove it.
| if (ferror(file)) { | ||
| die("cannot read filter"); | ||
| } | ||
| if (num_read != len_bytes) { |
| die("short read for filter %zu != %i", num_read, len_bytes); | ||
| } | ||
|
|
||
| free(prog->filter); // Add this line to free the allocated memory |
There was a problem hiding this comment.
This is not correct for several reasons:
- the next functions rely on this data being available
- freeing will make for a dangling pointer
- why bother with all the code above to just discard data just loaded
|
The function is declared as void, so returning NULL is invalid. Instead, you could set an error flag or call exit(EXIT_FAILURE) if terminating the program is appropriate. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
function sc_must_read_filter_from_file is changed to be checked against memory length validation